Stream the agent loop end to end and add markdown skills#6
Conversation
A Skill is a folder holding a SKILL.md: YAML frontmatter (name, description, and optional tool entry points) followed by a markdown instruction body. Skill.from_path parses and validates it; load_tools resolves declared "module:attribute" entry points to Tool objects. Adds pyyaml and a code-reviewer example skill.
Drive each model turn through model.stream and agglomerate_deltas, emitting TextDelta as tokens arrive. Register skills with progressive disclosure: only names and descriptions go in the system prompt, and the built-in load_skill tool reveals a skill's body and registers its tools on demand. Also fold in three review fixes: - Guard max_steps < 1 instead of silently falling back to the default. - Reject duplicate tool names at construction. - Add run() overloads: stream=False types as Awaitable[str], stream=True as AsyncIterator[Event].
PR Summary by QodoStream agent loop end-to-end and add progressive markdown skills
AI Description
Diagram
High-Level Assessment
Files changed (7)
|
Code Review by Qodo
1. Skill tools type coercion
|
| try: | ||
| name = frontmatter["name"] | ||
| description = frontmatter["description"] | ||
| except KeyError as exc: | ||
| raise ValueError( | ||
| f"{folder / SKILL_FILE} is missing required frontmatter key {exc}." | ||
| ) from exc | ||
|
|
||
| return cls( | ||
| name=name, | ||
| description=description, | ||
| instructions=body.strip(), | ||
| path=folder, | ||
| tools=list(frontmatter.get("tools") or []), | ||
| ) |
There was a problem hiding this comment.
1. Skill tools type coercion 🐞 Bug ≡ Correctness
Skill.from_path() coerces frontmatter.tools via list(...), so a YAML scalar string becomes a list of characters and later load_tools() feeds invalid specs into _resolve_tool(), breaking skill loading. name/description/tools also aren’t type-validated, so non-string YAML values can create hard-to-debug runtime failures (e.g., skill lookup mismatches).
Agent Prompt
### Issue description
`Skill.from_path()` does not validate the types of `name`, `description`, or `tools` from YAML frontmatter, and it normalizes `tools` with `list(frontmatter.get("tools") or [])`. If `tools` is provided as a YAML scalar (string), `list(str)` produces a list of characters; subsequent `load_tools()` calls `_resolve_tool()` with invalid one-character specs and the skill fails to load.
### Issue Context
Skills are intended to be user-authored markdown files. YAML commonly allows either scalars or sequences, so this failure mode is plausible and will surface as runtime errors when the model calls `load_skill()`.
### Fix Focus Areas
- src/agentling/skills.py[43-64]
- src/agentling/skills.py[66-109]
### Implementation notes
- Validate `name` and `description` are `str` (raise `ValueError` with file context if not).
- Normalize `tools` robustly:
- `None`/missing -> `[]`
- `str` -> `[tools_value]`
- `list`/`tuple` -> ensure all entries are `str`
- otherwise -> `ValueError`
- Consider adding tests for `tools: some.module:tool` (scalar) and for non-string `name/description` values.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| self.skills: dict[str, Skill] = { | ||
| skill.name: skill for skill in (_as_skill(entry) for entry in skills) | ||
| } |
There was a problem hiding this comment.
2. Skills silently overwritten 🐞 Bug ≡ Correctness
Agent.__init__ stores skills in a dict keyed by skill.name without checking duplicates, so later skills silently overwrite earlier ones. This can cause the agent to load a different skill than the caller intended when duplicate names exist.
Agent Prompt
### Issue description
`Agent.__init__` builds `self.skills` with a dict comprehension keyed by `skill.name`, which silently overwrites earlier skills when duplicate names are provided. This is inconsistent with the new behavior for tools (which explicitly rejects duplicate tool names) and can lead to confusing, incorrect skill resolution.
### Issue Context
Skill names are part of the system prompt catalog and are used as the lookup key for the built-in `load_skill(name)` tool. Silent overwrites make debugging configuration errors difficult.
### Fix Focus Areas
- src/agentling/agent.py[86-96]
### Implementation notes
- Replace the dict comprehension with a loop that checks for duplicates and raises `ValueError(f"Duplicate skill name: {name!r}")`.
- Add/adjust tests to cover providing two skills with the same name.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| deltas: list[Delta] = [] | ||
| async for delta in self.model.stream(messages, tools=self._tool_schemas): | ||
| if delta.content: | ||
| yield TextDelta(text=delta.content) | ||
| deltas.append(delta) | ||
|
|
||
| response = agglomerate_deltas(deltas) | ||
|
|
||
| # Forgiving termination: no tool calls means the content is the answer. | ||
| if not response.tool_calls: |
There was a problem hiding this comment.
3. Empty stream silent answer 🐞 Bug ☼ Reliability
If Model.stream() yields zero deltas, _run_stream() passes an empty list to agglomerate_deltas(), producing an assistant message with empty content and no tool calls; the agent then terminates with an empty final answer. This fails silently instead of surfacing a model/transport failure.
Agent Prompt
### Issue description
`Agent._run_stream()` assumes `Model.stream()` yields at least one delta. If it yields none, `agglomerate_deltas([])` returns a `ChatMessage` with `content=""` and `tool_calls=[]`, which the agent interprets as a valid terminal answer and returns an empty response.
### Issue Context
The `Model.stream()` protocol does not guarantee non-empty output. Empty output can occur due to provider bugs, early connection termination, or an adapter implementation mistake.
### Fix Focus Areas
- src/agentling/agent.py[188-206]
- src/agentling/agent.py[254-274]
- src/agentling/models.py[375-417]
### Implementation notes
- After the streaming loop, check `if not deltas:` and raise a clear exception (e.g., `RuntimeError("Model.stream produced no deltas")`) or otherwise surface an explicit failure.
- Apply the same guard to the step-limit forced-answer streaming block.
- Add a unit test with a Model.stream implementation that yields nothing and assert the agent raises (or emits a clear error outcome).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Day 6: streaming plus a progressive-disclosure skill system.
Streaming
model.streamand rebuilds theturn with
agglomerate_deltas, emittingTextDeltaas tokens arrive (boththe main step and the step-limit forced answer).
print_events(): a small live CLI renderer for the event stream.Skills
skills.py: aSkillis a folder with aSKILL.md(YAML frontmatter plus amarkdown body).
Skill.from_pathparses and validates it;load_toolsresolves declared
module:attributeentry points toToolobjects.prompt. The built-in
load_skill(name)tool reveals a skill's body andregisters its tools on demand, so context stays small until a skill is used.
code-reviewerexample skill underexamples/skills/.Review fixes folded in
max_steps < 1instead of silently using the default.run()overloads:stream=Falsetypes asAwaitable[str],stream=TrueasAsyncIterator[Event].33 new tests (98 total); mypy and ruff clean.
Closes FOL-43.